Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libmsquic 2.4.4 (new formula) #192082

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

liveans
Copy link

@liveans liveans commented Sep 27, 2024

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@github-actions github-actions bot added the new formula PR adds a new formula to Homebrew/homebrew-core label Sep 27, 2024
@liveans liveans changed the title libmsquic 2.4.4 libmsquic 2.4.4 (new formula) Sep 27, 2024
@liveans
Copy link
Author

liveans commented Sep 27, 2024

First time contributing into this repo, please let me know if I'm doing something wrong or missing anything obvious. Thanks!

@liveans liveans marked this pull request as ready for review September 27, 2024 09:49
Formula/lib/libmsquic.rb Outdated Show resolved Hide resolved
Comment on lines 4 to 6
url "https://github.com/microsoft/msquic.git",
tag: "v2.4.4",
revision: "e87ae9a37bcebdf5ab3725e5d882ddb50e4dbbb4"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
url "https://github.com/microsoft/msquic.git",
tag: "v2.4.4",
revision: "e87ae9a37bcebdf5ab3725e5d882ddb50e4dbbb4"
url "https://github.com/microsoft/msquic.git",
tag: "v2.4.4",
revision: "e87ae9a37bcebdf5ab3725e5d882ddb50e4dbbb4"

Do we need a Git clone? Can we use a tarball?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency and simplicity yes, we need git clone but it will be bound to tag/versions all the time, using tarball makes it harder to maintain it because it has some submodules.

Formula/lib/libmsquic.rb Outdated Show resolved Hide resolved
-DQUIC_BUILD_PERF=off
-DQUIC_BUILD_TOOLS=off
-DQUIC_BUILD_TESTS=off
-DHOMEBREW_ALLOW_FETCHCONTENT=on
Copy link
Author

@liveans liveans Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't avoid this right now, this requires upstream changes in the project and not sure if msquic is willing to do that.
FetchContent is only used to Fetch quictls here from submodules which is already fetched:
https://github.com/microsoft/msquic/blob/a09a6de23d724dc7cae9b5bf9dfb82205331a59a/CMakeLists.txt#L662-L678

Copy link
Contributor

@bugdea1er bugdea1er Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per CMake documentation:

If the dependency has already been populated earlier in this run, set the <lowercaseName>_POPULATED, <lowercaseName>_SOURCE_DIR, and <lowercaseName>_BINARY_DIR variables in the same way as a call to FetchContent_GetProperties(), then skip the remaining steps below and move on to the next dependency in the list.

https://cmake.org/cmake/help/latest/module/FetchContent.html#command:fetchcontent_makeavailable

Can we use this to avoid fetching submodules again?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, note that I'm just a bystander and not a Homebrew maintainer, better ask @carlocab if this FetchContent thing is important for Homebrew

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use this to avoid fetching submodules again?

We definitely can, but not sure what will be the response from msquic to this, even if we can make it happen on main, it's highly unlikely that it's going to get backported to 2.4 release and make a new release for this.

Copy link
Contributor

@bugdea1er bugdea1er Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I mean these variables can be set during CMake configuration step, without updating the sources

Copy link
Contributor

@bugdea1er bugdea1er Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QUIC_TLS should be set, CMake interpreter goes in the if(QUIC_TLS STREQUAL "openssl" OR QUIC_TLS STREQUAL "openssl3") condition, but does not populate the submodule because of the variables you provided

You still need to provide QUIC_TLS, and CMake should stumble upon FetchContent, but consider it already populated

Theoretically

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically yes, but after I set those variables as populated I still got the same error as

Refusing to populate dependency 'OpenSSLQuic' with FetchContent while
  building in Homebrew, please use a formula dependency or add a resource to
  the formula.

Copy link
Contributor

@bugdea1er bugdea1er Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what brew maintainers will say, but I suggest leaving HOMEBREW_ALLOW_FETCHCONTENT=ON but still using <lowercaseName>_POPULATED variables, so FetchContent doesn't download anything

Looks like brew's trapper was not ready for this scenario

@carlocab what do you think?

Copy link
Author

@liveans liveans Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that, but here is what's actually happening (if I understood correctly) submodules are already getting downloaded as part of git submodules, and we're not downloading anything with FetchContent, what FetchContent doing is just building quictls and make it a usable library with add_library. See:
https://github.com/microsoft/msquic/blob/a09a6de23d724dc7cae9b5bf9dfb82205331a59a/submodules/CMakeLists.txt#L10

Also, setting those variables leaves us to compilation step of quictls as part of brew formula as well, unless there is a way to add dependency of quictls to brew formula and get binary/include paths for this purpose.

I've talked with msquic team, and they will be happy to accept improvements for next release, but we still need workaround for release/2.4.x.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found similar case:

# We bypass brew's dependency provider to set `FETCHCONTENT_TRY_FIND_PACKAGE_MODE`
# which redirects FetchContent_Declare() to find_package() and helps find our `fmt`.
# To re-block fetches, we use the not-recommended `FETCHCONTENT_FULLY_DISCONNECTED`.
args = std_cmake_args + %W[
-DCMAKE_MODULE_LINKER_FLAGS=-Wl,-rpath,#{rpath(source: mlx_python_dir)}
-DHOMEBREW_ALLOW_FETCHCONTENT=ON
-DFETCHCONTENT_FULLY_DISCONNECTED=ON
-DFETCHCONTENT_TRY_FIND_PACKAGE_MODE=ALWAYS
-DFETCHCONTENT_SOURCE_DIR_GGUFLIB=#{buildpath}/gguflib

and setting these two parameters
-DFETCHCONTENT_FULLY_DISCONNECTED=ON -DFETCHCONTENT_TRY_FIND_PACKAGE_MODE=ALWAYS

along with -DHOMEBREW_ALLOW_FETCHCONTENT=ON, should do what we want, if I understood correctly, it will block any download over FetchContent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new formula PR adds a new formula to Homebrew/homebrew-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants